Conversation
call_tool_streaming failed for several protocols instead of emitting the full result as a single chunk like the HTTP protocol does: - CLI raised NotImplementedError on every streaming call. - MCP yielded the un-awaited coroutine instead of the result. - TCP was a plain coroutine returning a generator, so the client's `async for` failed. UDP gets the same shape and type annotation. SSE improvements: - Implement `reconnect` / `retry_timeout`, which were accepted but never acted on. When an established stream drops, reconnect after `retry_timeout` (or the server's `retry:` value) with `Last-Event-ID`, capped at MAX_RECONNECT_ATTEMPTS per call. A clean end of stream completes the call; connection or HTTP errors on the initial request still fail immediately. Redirects stay refused on every attempt. - Handle CRLF line endings and a trailing unterminated event. Tests added for every fixed protocol and for the SSE reconnect paths. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
mcp 2.x removed `mcp.server.fastmcp.FastMCP` and `mcp.shared.exceptions.McpError`, which the MCP test mocks import. CI installs the newest mcp, so every job failed at collection with mcp 2.1.1 (the last green run on dev predates the mcp 2 release). The plugin targets the 1.x API; a fresh install with the pin resolves to mcp 1.29.1 and the MCP suite passes. Migrating to mcp 2 is a separate task. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…nwrap Python counterpart of typescript-utcp PR #33 plus its follow-ups, so both SDKs behave the same in the next release. - Stdio MCP children no longer inherit the host's stderr. mcp-use's MCPClient.from_dict offers no way to set the errlog that StdioConnector hands to the SDK's stdio_client, so a thin MCPClient subclass sets the connector's errlog to os.devnull between construction and initialization. UTCP_MCP_CHILD_STDERR=inherit restores the old behavior for debugging (same switch as the TypeScript SDK), and a stdio server that fails to start logs a hint pointing at it. - structuredContent is used when it is not None (the previous hasattr check was always true on CallToolResult). A FastMCP-style single-key {"result": value} wrapper is unwrapped; an object that merely has a "result" key among others is a genuine object return and now passes through untouched instead of losing its sibling keys. - Tests for both, plus a README section on child process stderr. Circular $ref handling from #33 needs no port: this plugin passes MCP schemas through without dereferencing them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
mcp 2.x removed `mcp.server.fastmcp.FastMCP` and `mcp.shared.exceptions.McpError`, which the MCP test mocks import. CI installs the newest mcp, so every job failed at collection with mcp 2.1.1 (the last green run on dev predates the mcp 2 release). The plugin targets the 1.x API; a fresh install with the pin resolves to mcp 1.29.1 and the MCP suite passes. Migrating to mcp 2 is a separate task. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
FastMCP wraps only non-object returns as {"result": value}, so a
single-key {"result": {...}} is a genuine object return and must keep its
shape. Unwrap only when the inner value is not a dict. Tests added for the
list wrapper and the genuine single-key object return. Mirrors the cubic
review fix on typescript-utcp#42.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ys, fix CRLF framing Addresses cubic review on #100: - The handshake (until response headers arrive) is bounded by HANDSHAKE_TIMEOUT_SECONDS (30 s) via asyncio.wait_for, so a server that accepts the connection but never answers cannot hang the call. Reading the body stays unbounded: an SSE stream may legitimately be quiet. - A reconnect handshake that fails (refused, 503, timeout) now counts as one attempt and is retried; only the initial handshake fails fast. - The reconnect delay is capped at MAX_RECONNECT_DELAY_MS (60 s) whatever retry_timeout or a server-sent retry: field asks for, so the attempt cap actually bounds the total wait. - A CRLF split across two chunks no longer becomes two LFs and ends the event early: a trailing CR is held until the next chunk. Decoding is now incremental too, so a multi-byte UTF-8 character straddling chunks no longer raises. - An event that exceeds MAX_EVENT_BUFFER_CHARS (16 Mi) without a blank-line delimiter raises SseProtocolError, which is never retried. Tests for each of the above. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Count connections in the no-delimiter handler and assert exactly one, so the test actually verifies that SseProtocolError bypasses the reconnect path instead of relying on the error being re-raised on a retry. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…aming-single-chunk-and-sse-reconnect Fix streaming mode for CLI, MCP, TCP protocols and add SSE reconnection
Both #100 and this branch appended tests to test_mcp_transport.py; keep both.
…quiet-child-stderr mcp: quiet stdio child stderr by default, tighten structuredContent unwrap
Python counterpart of typescript-utcp #26 / #44. raise_for_status() raises a ClientResponseError whose message is only the reason phrase ("Forbidden"); the body, where servers put the real reason ({"error": "..."}), was discarded, so a refused call or discovery surfaced as nothing more than a status code. New utcp_http._errors.raise_for_status_with_body reads the body on a 4xx/5xx and raises a ClientResponseError of the same status and headers whose message is "<reason>: <detail>", where detail is a string error / message / detail field when the body is JSON, otherwise the raw body (truncated). The raw text is attached as .body. Used by the HTTP protocol's tool calls and discovery and by SSE and Streamable HTTP discovery. The exception type is unchanged, so existing handlers keep working. Tests: body in the call error, object-valued error field shows its structure, and discovery errors[] carries the body for all three protocols. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ounded Addresses cubic review on #102: - error_detail_from_body no longer skips an object-valued `error` to reach a lower-priority generic string: the first of error / message / detail that is present decides, and a structured value returns the raw JSON so its shape stays visible. Null and blank strings are still skipped. - The error body is read incrementally and capped at MAX_BODY_READ_BYTES (64 KiB) instead of buffered in full and truncated afterwards, so an arbitrarily large 4xx/5xx body from an untrusted endpoint cannot grow memory unbounded. Decoding is lenient and honours the response charset. Tests for precedence, null/blank skipping, and a 1 MiB error body. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…d detail - Decoding a bounded error body now falls back to UTF-8 when the response declares an unknown charset (LookupError) or the lookup fails for any other reason, so the detail is never lost. Tests for a body without a Content-Type and one with an unknown charset. - The precedence test asserts on the parsed detail instead of slicing the message string. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…-surface-error-body http: surface the server's error body on failed calls and discovery
… streaming calls Pre-release review of dev: - mcp: _ensure_mcp_client compared the client's whole config dict with its mcpServers entry, which was always unequal, so every tool call built a new MCPClient and spawned a fresh server process that was never closed. Compare the mcpServers entry, and close the previous client's sessions when the configuration really changes. Test asserts one client and one session across two calls. - sse / streamable_http: the streaming call paths now raise with the server's error body like discovery and the TypeScript SDK already do. - _errors: decode the bounded body once. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 task
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 17 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="plugins/communication_protocols/http/tests/test_sse_communication_protocol.py">
<violation number="1" location="plugins/communication_protocols/http/tests/test_sse_communication_protocol.py:154">
P3: slow_handshake_handler sleeps a hard-coded 5s, but test_initial_handshake_timeout_raises patches HANDSHAKE_TIMEOUT_SECONDS to 0.3, so the client aborts the handshake at ~0.3s while the server-side handler coroutine keeps sleeping ~4.7s longer. After the test returns the handler is still pending on the event loop, and when it wakes it writes a 204 to a client that already disconnected. This leaves a dangling task that can emit "Task was destroyed but it is pending" noise and slow teardown. Reduce the sleep so it finishes near the timeout, or sleep until the transport is closed instead of a fixed 5s.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| async def slow_handshake_handler(request): | ||
| """Accepts the connection but does not send response headers for a long time.""" | ||
| await asyncio.sleep(5) |
Contributor
There was a problem hiding this comment.
P3: slow_handshake_handler sleeps a hard-coded 5s, but test_initial_handshake_timeout_raises patches HANDSHAKE_TIMEOUT_SECONDS to 0.3, so the client aborts the handshake at ~0.3s while the server-side handler coroutine keeps sleeping ~4.7s longer. After the test returns the handler is still pending on the event loop, and when it wakes it writes a 204 to a client that already disconnected. This leaves a dangling task that can emit "Task was destroyed but it is pending" noise and slow teardown. Reduce the sleep so it finishes near the timeout, or sleep until the transport is closed instead of a fixed 5s.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/communication_protocols/http/tests/test_sse_communication_protocol.py, line 154:
<comment>slow_handshake_handler sleeps a hard-coded 5s, but test_initial_handshake_timeout_raises patches HANDSHAKE_TIMEOUT_SECONDS to 0.3, so the client aborts the handshake at ~0.3s while the server-side handler coroutine keeps sleeping ~4.7s longer. After the test returns the handler is still pending on the event loop, and when it wakes it writes a 204 to a client that already disconnected. This leaves a dangling task that can emit "Task was destroyed but it is pending" noise and slow teardown. Reduce the sleep so it finishes near the timeout, or sleep until the transport is closed instead of a fixed 5s.</comment>
<file context>
@@ -105,6 +105,91 @@ async def token_header_auth_handler(request):
+
+async def slow_handshake_handler(request):
+ """Accepts the connection but does not send response headers for a long time."""
+ await asyncio.sleep(5)
+ return web.Response(status=204)
+
</file context>
- sse: an event block without an `event:` field has the type "message" (spec), so event_type="message" now matches it. An empty `id:` resets the last event ID and no Last-Event-ID header is sent for it; ids containing NUL are ignored. A 200 whose Content-Type is not text/event-stream raises SseProtocolError instead of parsing into zero events. Calls that send a request body are never reconnected: a re-issued POST could re-execute a non-idempotent tool. - _errors: a body nested deeper than the JSON parser's recursion limit raised RecursionError past the HTTP error handling; it is caught and treated as text. Control characters are collapsed so a server cannot forge log records or terminal escapes through an error message. - mcp: close() referenced a _session_locks attribute that was never assigned and always raised AttributeError after cleanup. A child that starts but fails the MCP handshake is now disconnected and removed from active_sessions instead of lingering. Tests for each. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- sse: `retry:` is honoured only when made of ASCII digits (spec); "-1" or "20ms" no longer change the reconnect delay. A stream that ends in the middle of an event no longer dispatches the incomplete event (spec: pending data is discarded at end of file). - streamable_http: Content-Type is matched case-insensitively. - udp: remove the stale comment block describing the bug this release fixed. - tests: the slow-handshake handler no longer outlives the test by seconds. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Member
Author
|
Disposition of the 7 findings, all handled in #104 (targets dev, so this PR picks them up once it merges):
#104 also carries fixes from my own review: the MCP client is reused across calls instead of spawning a new server process per call (pre-existing), |
A substring check let a Content-Type such as text/event-stream-invalid through; compare the media-type portion exactly, parameters allowed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- mcp: one MCPClient per distinct server configuration instead of a single shared client. The protocol object is registered once per process and shared by every manual, so a single client made manuals with different configurations evict each other's sessions, including ones still in use by a concurrent call. Clients are keyed by the canonical configuration, created under a lock so two concurrent first calls cannot each spawn a server, and never evicted by another manual's activity. close() closes every client's sessions and keeps a client whose shutdown failed so a later close() can retry it. - sse: the error-body read on a refused stream is bounded by the handshake timeout, so a server that answers 4xx/5xx and then stalls cannot hang the call. A final blank line ending in a lone CR still completes the last event; only genuinely incomplete events are discarded. Absurdly long retry digit strings are ignored. - _errors: control-character collapsing covers the C1 range too. - tests: streaming calls against a 5xx assert the body is surfaced for both SSE and Streamable HTTP; separate-clients-per-configuration test; the malformed-retry test is named for what it verifies. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- mcp: when a manual's configuration changes and no other manual uses the old one, the old client's sessions are closed instead of lingering until close(). Ownership is tracked per manual name. - sse: the residual buffer is checked against the event cap at end of stream too; retry digit strings are bounded to 18 digits before conversion. - tests: the connection-drop handlers wait 100 ms after writing the first event before closing the socket. On Windows CI the event and the close otherwise arrived together and aiohttp raised before delivering the event, failing six tests that pass elsewhere. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- mcp: deregistering a manual drops only that manual's claim on its client; the client's sessions are closed when no manual references the configuration any more. Two manuals with identical configurations share one client, and deregistering one no longer tears down the other's sessions or leaves a ghost ownership entry. - tests: the streaming error-body tests use a body distinct from the reason phrase so they can only pass when the body is surfaced; deregistration test for shared configurations. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…me alone The protocol object is a process-wide singleton, so two UtcpClient instances may register a manual of the same name with different configurations; keyed by name alone, the second registration overwrote the first's ownership entry and closed its client. The calling client's identity is now part of the owner key, carried through the public entry points with a context variable rather than threading `caller` through every helper. Test added. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Restoring a failed client into the live map happened outside the lock and could overwrite a newer client for the same configuration. Failed clients now go to a separate list that close() retries. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…polish Pre-release fixes: reuse the MCP client across calls; error bodies on streaming calls
ensure_secure_url permits loopback HTTP for local development, which left hand-written UTCP manuals able to declare tool URLs on the agent's own loopback interface even when discovered from a remote origin. The OpenAPI converter already enforces this rule for specs it converts; apply the same check to native UTCP manuals in the http, sse and streamable_http protocols via a shared reject_remote_loopback_tool_urls helper. Manuals discovered from loopback (local dev) stay exempt. Adds unit tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Manual-level OAuth2 was accepted on the MCP call template but never used, so an auth block had no effect. Fetch the token and inject it as the connection's bearer credential for HTTP servers that don't already carry their own. Validate the OAuth2 token endpoint before sending credentials to it, and validate HTTP/WS server URLs before dialing, matching the trust boundary the HTTP-family plugins enforce. Key clients by auth as well as server config so manuals with distinct credentials don't share a client or token. Adds unit tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
5 issues found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="plugins/communication_protocols/http/src/utcp_http/_security.py">
<violation number="1" location="plugins/communication_protocols/http/src/utcp_http/_security.py:500">
P2: When a loopback discovery URL redirects to a remote manual, this exemption trusts the initial URL rather than the origin that supplied the manual. Track the final response URL through discovery and apply the loopback check to that URL, or reject cross-origin redirects from loopback discovery.</violation>
<violation number="2" location="plugins/communication_protocols/http/src/utcp_http/_security.py:505">
P1: This misses resolver-valid loopback aliases such as `https://127.1/...`: `is_loopback_url` returns false, while the invocation's HTTPS check allows the request and the resolver maps the host to `127.0.0.1`. Canonicalize numeric host forms or resolve and reject loopback destinations before allowing a remote manual.</violation>
<violation number="3" location="plugins/communication_protocols/http/src/utcp_http/_security.py:505">
P1: A remote manual can bypass this check with a templated authority such as `https://{host}/...`: the check runs before `{host}` is resolved, then the invocation path substitutes `127.0.0.1` and `ensure_secure_url` permits it. Reject dynamic hosts for remote manuals or retain the manual's remote trust state and repeat this check after URL substitution.</violation>
</file>
<file name="plugins/communication_protocols/mcp/src/utcp_mcp/mcp_communication_protocol.py">
<violation number="1" location="plugins/communication_protocols/mcp/src/utcp_mcp/mcp_communication_protocol.py:281">
P2: When a manual has OAuth2 metadata but no server needs the manual bearer token, registration still contacts the token endpoint and can fail before starting an otherwise valid stdio server. Fetch the token only when a URL server lacks `auth_token` and an `Authorization` header.</violation>
<violation number="2" location="plugins/communication_protocols/mcp/src/utcp_mcp/mcp_communication_protocol.py:284">
P1: When two manuals reuse a `client_id` with different token endpoints or secrets, this new path sends the first manual's cached bearer token to the second server. The same cache returns expiring tokens forever, so calls fail after token expiry; scope the cache by issuer/credentials and refresh tokens before reuse.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if isinstance(manual_call_template.auth, OAuth2Auth): | ||
| # Fetches (and validates the token endpoint of) the manual's OAuth2 | ||
| # credentials before any server connection is dialed. | ||
| token = await self._handle_oauth2(manual_call_template.auth) |
Contributor
There was a problem hiding this comment.
P1: When two manuals reuse a client_id with different token endpoints or secrets, this new path sends the first manual's cached bearer token to the second server. The same cache returns expiring tokens forever, so calls fail after token expiry; scope the cache by issuer/credentials and refresh tokens before reuse.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/communication_protocols/mcp/src/utcp_mcp/mcp_communication_protocol.py, line 284:
<comment>When two manuals reuse a `client_id` with different token endpoints or secrets, this new path sends the first manual's cached bearer token to the second server. The same cache returns expiring tokens forever, so calls fail after token expiry; scope the cache by issuer/credentials and refresh tokens before reuse.</comment>
<file context>
@@ -187,6 +267,37 @@ async def _ensure_mcp_client(self, manual_call_template: 'McpCallTemplate') -> M
+ if isinstance(manual_call_template.auth, OAuth2Auth):
+ # Fetches (and validates the token endpoint of) the manual's OAuth2
+ # credentials before any server connection is dialed.
+ token = await self._handle_oauth2(manual_call_template.auth)
+ for server_name, server_config in servers.items():
+ if not isinstance(server_config, dict):
</file context>
| for tool in getattr(manual, "tools", None) or []: | ||
| call_template = getattr(tool, "tool_call_template", None) | ||
| url = getattr(call_template, "url", None) | ||
| if isinstance(url, str) and is_loopback_url(url): |
Contributor
There was a problem hiding this comment.
P1: This misses resolver-valid loopback aliases such as https://127.1/...: is_loopback_url returns false, while the invocation's HTTPS check allows the request and the resolver maps the host to 127.0.0.1. Canonicalize numeric host forms or resolve and reject loopback destinations before allowing a remote manual.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/communication_protocols/http/src/utcp_http/_security.py, line 505:
<comment>This misses resolver-valid loopback aliases such as `https://127.1/...`: `is_loopback_url` returns false, while the invocation's HTTPS check allows the request and the resolver maps the host to `127.0.0.1`. Canonicalize numeric host forms or resolve and reject loopback destinations before allowing a remote manual.</comment>
<file context>
@@ -478,3 +478,35 @@ async def safe_request_with_redirects(
+ for tool in getattr(manual, "tools", None) or []:
+ call_template = getattr(tool, "tool_call_template", None)
+ url = getattr(call_template, "url", None)
+ if isinstance(url, str) and is_loopback_url(url):
+ raise ValueError(
+ f"Security error during {context}: a manual fetched from "
</file context>
| for tool in getattr(manual, "tools", None) or []: | ||
| call_template = getattr(tool, "tool_call_template", None) | ||
| url = getattr(call_template, "url", None) | ||
| if isinstance(url, str) and is_loopback_url(url): |
Contributor
There was a problem hiding this comment.
P1: A remote manual can bypass this check with a templated authority such as https://{host}/...: the check runs before {host} is resolved, then the invocation path substitutes 127.0.0.1 and ensure_secure_url permits it. Reject dynamic hosts for remote manuals or retain the manual's remote trust state and repeat this check after URL substitution.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/communication_protocols/http/src/utcp_http/_security.py, line 505:
<comment>A remote manual can bypass this check with a templated authority such as `https://{host}/...`: the check runs before `{host}` is resolved, then the invocation path substitutes `127.0.0.1` and `ensure_secure_url` permits it. Reject dynamic hosts for remote manuals or retain the manual's remote trust state and repeat this check after URL substitution.</comment>
<file context>
@@ -478,3 +478,35 @@ async def safe_request_with_redirects(
+ for tool in getattr(manual, "tools", None) or []:
+ call_template = getattr(tool, "tool_call_template", None)
+ url = getattr(call_template, "url", None)
+ if isinstance(url, str) and is_loopback_url(url):
+ raise ValueError(
+ f"Security error during {context}: a manual fetched from "
</file context>
Comment on lines
+281
to
+284
| if isinstance(manual_call_template.auth, OAuth2Auth): | ||
| # Fetches (and validates the token endpoint of) the manual's OAuth2 | ||
| # credentials before any server connection is dialed. | ||
| token = await self._handle_oauth2(manual_call_template.auth) |
Contributor
There was a problem hiding this comment.
P2: When a manual has OAuth2 metadata but no server needs the manual bearer token, registration still contacts the token endpoint and can fail before starting an otherwise valid stdio server. Fetch the token only when a URL server lacks auth_token and an Authorization header.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/communication_protocols/mcp/src/utcp_mcp/mcp_communication_protocol.py, line 281:
<comment>When a manual has OAuth2 metadata but no server needs the manual bearer token, registration still contacts the token endpoint and can fail before starting an otherwise valid stdio server. Fetch the token only when a URL server lacks `auth_token` and an `Authorization` header.</comment>
<file context>
@@ -187,6 +267,37 @@ async def _ensure_mcp_client(self, manual_call_template: 'McpCallTemplate') -> M
+ """
+ servers = copy.deepcopy(manual_call_template.config.mcpServers)
+ token: Optional[str] = None
+ if isinstance(manual_call_template.auth, OAuth2Auth):
+ # Fetches (and validates the token endpoint of) the manual's OAuth2
+ # credentials before any server connection is dialed.
</file context>
Suggested change
| if isinstance(manual_call_template.auth, OAuth2Auth): | |
| # Fetches (and validates the token endpoint of) the manual's OAuth2 | |
| # credentials before any server connection is dialed. | |
| token = await self._handle_oauth2(manual_call_template.auth) | |
| if ( | |
| isinstance(manual_call_template.auth, OAuth2Auth) | |
| and any( | |
| isinstance(server_config, dict) | |
| and "url" in server_config | |
| and not server_config.get("auth_token") | |
| and not _has_authorization_header(server_config) | |
| for server_config in servers.values() | |
| ) | |
| ): | |
| token = await self._handle_oauth2(manual_call_template.auth) |
| tool's call-template URL. A manual fetched from loopback (local dev) is | ||
| exempt, exactly as the converter exempts a local spec. | ||
| """ | ||
| if is_loopback_url(discovery_url): |
Contributor
There was a problem hiding this comment.
P2: When a loopback discovery URL redirects to a remote manual, this exemption trusts the initial URL rather than the origin that supplied the manual. Track the final response URL through discovery and apply the loopback check to that URL, or reject cross-origin redirects from loopback discovery.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/communication_protocols/http/src/utcp_http/_security.py, line 500:
<comment>When a loopback discovery URL redirects to a remote manual, this exemption trusts the initial URL rather than the origin that supplied the manual. Track the final response URL through discovery and apply the loopback check to that URL, or reject cross-origin redirects from loopback discovery.</comment>
<file context>
@@ -478,3 +478,35 @@ async def safe_request_with_redirects(
+ tool's call-template URL. A manual fetched from loopback (local dev) is
+ exempt, exactly as the converter exempts a local spec.
+ """
+ if is_loopback_url(discovery_url):
+ return
+ for tool in getattr(manual, "tools", None) or []:
</file context>
- The remote-loopback manual check keys off the final (post-redirect) discovery URL, so a loopback discovery URL that redirects to a remote origin loses the local-dev exemption. - The MCP OAuth2 token request no longer follows redirects and refuses a 3xx, so a token endpoint cannot bounce the credential POST to another host. - MCP token injection now preserves a server config's own auth field. - OAuth security tests no longer perform network I/O (cache-seeded / guard-only). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Parametrize loopback tool-URL rejection over 127.0.0.1, localhost, 127.0.0.0/8, 0.0.0.0 and IPv4-mapped forms, so the comment's claimed forms are actually exercised. - Add a loopback MCP server URL accept case and a case asserting a server config's own auth field is preserved rather than overwritten by the manual token. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The manual OAuth2 token fetch runs inside _build_connection_servers, which was awaited while holding _clients_lock. Since the token endpoint comes from the (untrusted) manual and its fetch is network I/O with no short timeout, a slow endpoint could hold the lock and stall client creation for every manual. Build the config before taking the lock; from_dict spawns no processes, so a config left unused after losing the creation race is inert. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Moving the token fetch outside _clients_lock let concurrent first-time callers for the same manual each POST to the token endpoint. Share a single in-flight fetch per client_id (keyed like the token cache) so one request runs and the other callers await its result; the fetch body moves to _fetch_oauth2_token. Adds a test asserting five concurrent calls trigger exactly one fetch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 2 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Awaiting the shared fetch task directly propagated a waiter's cancellation into the task, cancelling token acquisition for every other waiter. Await it through asyncio.shield so a cancelled waiter raises on its own while the shared fetch completes for the rest. Adds a test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Concurrent first-time calls for the same (configuration, server) each ran client.create_session, so all but the last session were orphaned and leaked. Share one shielded creation task per (config, server) keyed like the client map; the create logic moves to _create_session. Adds coalescing and cancellation tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="plugins/communication_protocols/mcp/src/utcp_mcp/mcp_communication_protocol.py">
<violation number="1" location="plugins/communication_protocols/mcp/src/utcp_mcp/mcp_communication_protocol.py:334">
P1: When a client is retired while its session creation is pending, a later client with the same configuration reuses this task even though `_create_session` is bound to the retired client. Scope in-flight creations to the client instance and cancel/await them before deregistration or protocol shutdown closes that client.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| # burst of first calls dials once instead of each spawning a session and | ||
| # leaking all but the last. The check-and-set is synchronous, so exactly | ||
| # one task is created. | ||
| inflight_key = (self._config_key(manual_call_template), server_name) |
Contributor
There was a problem hiding this comment.
P1: When a client is retired while its session creation is pending, a later client with the same configuration reuses this task even though _create_session is bound to the retired client. Scope in-flight creations to the client instance and cancel/await them before deregistration or protocol shutdown closes that client.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plugins/communication_protocols/mcp/src/utcp_mcp/mcp_communication_protocol.py, line 334:
<comment>When a client is retired while its session creation is pending, a later client with the same configuration reuses this task even though `_create_session` is bound to the retired client. Scope in-flight creations to the client instance and cancel/await them before deregistration or protocol shutdown closes that client.</comment>
<file context>
@@ -314,27 +318,45 @@ async def _build_connection_servers(self, manual_call_template: 'McpCallTemplate
+ # burst of first calls dials once instead of each spawning a session and
+ # leaking all but the last. The check-and-set is synchronous, so exactly
+ # one task is created.
+ inflight_key = (self._config_key(manual_call_template), server_name)
+ task = self._session_creations.get(inflight_key)
+ if task is None:
</file context>
…n to the client instance The token cache and in-flight fetch map were keyed by client_id alone, so two manuals sharing a client_id but differing in token URL, secret or scope received each other's tokens. Key both by the full OAuth configuration, matching the HTTP plugin and the TypeScript fix. In-flight session creations are now keyed by client identity rather than configuration, so a client retired while a creation is pending cannot hand that task (bound to the retired client) to a later same-config client. Tests updated. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A drain left credentials cached on this shared instance, unlike the TypeScript plugin. Clear the token cache in close(); in-flight fetches are left to self-prune when they settle. Adds a test. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…ending fetches on close() The fetch wrote the cache unconditionally, so a fetch still in flight when close() ran repopulated the cache afterwards and the drain did not actually leave the instance credential-free. Apply the same invariant as the TypeScript plugin: the in-flight entry is the sole authority for caching. The settle handler caches a result only if its task is still the current entry, and removes the entry only then; _fetch_oauth2_token is now a pure fetch returning the token response. close() cancels in-flight fetches and drops their entries, so a fetch that still lands is no longer current and does not cache. Adds a post-close regression test; the coalescing stubs now return the token response for the handler to cache. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
A 200 whose body lacks access_token was returned as a success, cached by the settle handler, and then failed on the read path — and since the cache is only ever cleared by a drain, one malformed reply became a persistent OAuth failure. Validate at the fetch boundary instead of guarding the cache write: _require_access_token turns a malformed body into a ClientError, so the cache only ever receives validated responses, the body-vs-Basic fallback proceeds exactly as it would on a transport error, and callers get a real error rather than a KeyError. Matches the TypeScript plugin. Also fixes the post-close test so it actually exercises the identity gate: the fake fetch now signals it is running (inside its try) before the drain, since cancelling a not-yet-started coroutine throws at entry and would only ever test cancellation. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Completes the token-response validation predicate at the fetch boundary. It accepted any truthy access_token, so a number, True or an object would pass, be cached, and be injected as an invalid bearer credential on every reuse. A usable token is a non-empty string; anything else is a failed fetch and never reaches the cache. The test now covers the non-string cases, which are exactly what fails if the string requirement is removed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…le ASCII Replaces a growing denylist of bad token shapes with the rule derived from the contract the token must satisfy: mcp-use places it verbatim into an Authorization: Bearer header, and a header value may contain only visible ASCII (RFC 9110 VCHAR, 0x21-0x7E), with a space ending the token. One rule makes every unusable shape inexpressible at once, including CR/LF header injection, NUL and non-ASCII. RFC 6750's narrower b64token alphabet was deliberately not used: it would reject legitimate opaque tokens. Tests cover each unusable shape (each fails if the VCHAR clause is removed) and a printable-punctuation token that must be accepted. Parity with typescript-utcp. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
utcp-cli 1.1.4 -> 1.1.5, utcp-http 1.1.11 -> 1.1.12, utcp-mcp 1.1.2 -> 1.1.3, utcp-socket 1.1.0 -> 1.1.1. Patch bumps: every change is a backwards-compatible bug or security fix. Core is unchanged and not bumped. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by cubic
Turns protocol streaming and discovery paths into reliable, bounded operations instead of leaving unsupported generators, unbounded reconnects, or opaque failures. It also tightens endpoint and MCP authentication checks while preserving local loopback development.
Bug Fixes
client_idbut differ in token URL, secret, or scope cannot receive each other's tokens.close()now clears cached OAuth tokens and cancels in-flight fetches so they cannot repopulate the cache on the shared instance.access_tokenis not a non-empty visible-ASCII string fail instead of being cached.Last-Event-ID, retries at most 5 times with 60-second delays, and bounds handshakes to 30 seconds; initial failures still fail fast and POST streams are not replayed.error/message/detailbodies.Dependencies
utcp_mcppinsmcpto<2becausemcp2 removed APIs the plugin uses.UTCP_MCP_CHILD_STDERR=inheritto restore it while debugging.utcp-cli,utcp-http,utcp-mcp, andutcp-socketget patch version bumps.Written for commit 961e246. Summary will update on new commits.